Add EventEmitter to XCM executor. #7234

jpserrat · 2025-01-18T14:05:06Z

@franciscoaguirre I added the EventEmitter trait to the config. I have two questions on this:

I created a new file to put the trait in, is this ok?
For EventEmitter trait I've created a sent event function instead a generic one that emits any event, do you prefer a generic one?

… send

…rrat/add-sent-event

bkontur · 2025-01-20T13:45:28Z

polkadot/xcm/xcm-executor/src/lib.rs

@@ -427,9 +428,20 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			reason = ?reason,
 			"Sending msg",
 		);
-		let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?;
+		let (ticket, fee) = validate_send::<Config::XcmSender>(dest.clone(), msg.clone())?;


Just thinking out loud, do we really want to log/use msg.clone here?
My concern is that some Config::XcmSender / SendXcm implementations might modify the msg, such as WithUniqueTopic. This could lead to confusion, as the msg in the event might differ from the one actually sent (or received on the other side).

definitely don't include full message in event, see my comment in the trait definition

bkontur · 2025-01-20T13:46:16Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+
+use xcm::latest::prelude::*;
+
+pub trait EventEmitter {


please add some documentation here at least

bkontur · 2025-01-20T13:51:49Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+use xcm::latest::prelude::*;
+
+pub trait EventEmitter {
+    fn emit_sent_event(


The emit_sent_event function, with Self::deposit_event(Event::Sent, makes it a single-purpose solution.

I would prefer a more general approach. For example, there’s a very relevant issue where we want/need to emit a failure event from ProcessXcmMessage.

If we could address this issue using an EventEmitter, that would be fantastic.

one option is this trait to have multiple functions, each specific to xcm-executor events

fn emit_sent_event is one
fn emit_process_event is another
etc

acatangiu · 2025-01-20T14:28:16Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+use xcm::latest::prelude::*;
+
+pub trait EventEmitter {
+    fn emit_sent_event(


one option is this trait to have multiple functions, each specific to xcm-executor events

fn emit_sent_event is one
fn emit_process_event is another
etc

acatangiu · 2025-01-20T14:29:22Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+    fn emit_sent_event(
+        origin: Location,
+        destination: Location,
+        message: Xcm<()>,


Suggested change

message: Xcm<()>,

definitely cannot include full message in Events, on chain storage should be kept to a minimum

providing the message_id: XcmHash should be enough

Should we remove this from the other events? In pallet xcm we are emitting this sent event with the message.

no, do not change any existing events, we don't know how indexers or exchanges exactly depend on them and we risk breaking something

acatangiu · 2025-01-20T14:30:26Z

polkadot/xcm/xcm-executor/src/lib.rs

@@ -46,6 +46,7 @@ mod assets;
 pub use assets::AssetsInHolding;
 mod config;
 pub use config::Config;
+use crate::traits::EventEmitter;


move this to the use traits::{... above

acatangiu · 2025-01-20T14:30:56Z

polkadot/xcm/xcm-executor/src/lib.rs

@@ -427,9 +428,20 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			reason = ?reason,
 			"Sending msg",
 		);
-		let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?;
+		let (ticket, fee) = validate_send::<Config::XcmSender>(dest.clone(), msg.clone())?;


definitely don't include full message in event, see my comment in the trait definition

acatangiu · 2025-01-20T14:32:20Z

polkadot/xcm/xcm-executor/src/lib.rs

-		Config::XcmSender::deliver(ticket).map_err(Into::into)
+		let message_id = match Config::XcmSender::deliver(ticket) {
+			Ok(message_id) => message_id,
+			Err(e) => return Err(e.into()),


should emit event for error too

As we are going to send a event for the process failure, should we just send the process failure event or send both event on the send failure and process failure?

ideally, the process failure event would clearly describe the failure as a send failure

if not possible/easy, an extra explicit send failure event is also ok

acatangiu · 2025-01-20T14:35:10Z

polkadot/xcm/pallet-xcm/src/lib.rs

+		message: Xcm<()>,
+		message_id: XcmHash,
+		) {
+			Self::deposit_event(Event::Sent { origin, destination, message, message_id });


would not reuse Event::Sent which is used by pallet_xcm::send()

Suggested change

Self::deposit_event(Event::Sent { origin, destination, message, message_id });

Self::deposit_event(Event::XcmExecutorSent { origin, destination, message_id });

Why would you not reuse it? Are they really that different?
If they're just one, apps can just subscribe to one event and know all cross-chain messages from that.

true, on second thought we could reuse, I am still not sure about including the full message in the event though, some messages could be quite big

acatangiu · 2025-02-10T07:40:34Z

@jpserrat do you plan on finishing this PR or should we take over?

jpserrat · 2025-02-10T09:16:50Z

@acatangiu Sorry for the delay on this. I'm going to finish.

raymondkfcheung · 2025-02-11T10:05:06Z

If #7200 is merged before your PR, you may need add the new EventEmitter on the XcmTestConfig.

polkadot/xcm/pallet-xcm/src/lib.rs

raymondkfcheung · 2025-02-13T14:56:45Z

polkadot/xcm/xcm-executor/src/lib.rs

@@ -813,6 +831,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 						self.process_instruction(instr)
 					});
 					if let Err(e) = inst_res {
+						Config::XcmEventEmitter::emit_process_failure_event(self.original_origin.clone(), e.clone());


Move this line, emit_process_failure_event, below, the tracing. Then we can see the trace, even it fails to emit.

raymondkfcheung · 2025-02-13T15:03:20Z

polkadot/xcm/pallet-xcm/src/lib.rs

+		/// A XCM message failed to be sent.
+		SentFailed { origin: Location, destination: Location, error: SendError },
+		/// Process XCM message failed.
+		ProcessXcmError { origin: Location, error: XcmError },


These two new events should include, message_id, for better message tracking.

For these ones when the event is emitted we don't have the message id. This was something that I was in doubt when creating this two events. Do you have any suggestion on what we can add to these events?

Is possible to retrieve message_id from SetTopic or properties.message_id?

raymondkfcheung · 2025-02-13T15:03:47Z

polkadot/xcm/pallet-xcm/src/lib.rs

+		fn emit_sent_failure_event(
+			origin: Location,
+			destination: Location,
+			error: SendError,
+		) {
+            Self::deposit_event(Event::SentFailed { origin, destination, error });
+        }
+
+		fn emit_process_failure_event(
+			origin: Location,
+			error: XcmError,
+		) {
+			Self::deposit_event(Event::ProcessXcmError { origin, error });
+		}


These two functions are not tested.

raymondkfcheung · 2025-02-13T15:05:53Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+        origin: Location,
+        destination: Location,
+        message_id: XcmHash,


Can we use references like origin: &Location, destination: &Location, message_id: &XcmHash? Similar to other two functions.

Also, please include the message.

raymondkfcheung · 2025-02-13T15:08:24Z

polkadot/xcm/xcm-executor/src/traits/event_emitter.rs

+    fn emit_sent_failure_event(
+        origin: Location,
+        destination: Location,
+        error: SendError,
+    );
+    fn emit_process_failure_event(
+        origin: Location,
+        error: XcmError
+    );


Please include message_id.

jpserrat added 2 commits January 18, 2025 10:38

feat: add event emitter to xcm executor config and emit sent event on…

d045373

… send

Merge branch 'master' of github.com:paritytech/polkadot-sdk into jpse…

2cece4e

…rrat/add-sent-event

jpserrat requested a review from a team as a code owner January 18, 2025 14:05

paritytech-review-bot bot requested a review from a team January 18, 2025 14:06

bkontur reviewed Jan 20, 2025

View reviewed changes

bkontur mentioned this pull request Jan 20, 2025

Add "Sent" event when a message is sent via execute #6851

Open

acatangiu reviewed Jan 20, 2025

View reviewed changes

franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Jan 28, 2025

franciscoaguirre self-assigned this Jan 28, 2025

feat: send failure events and add doc to event emiter

4ecc682

paritytech-review-bot bot requested a review from a team February 12, 2025 21:20

raymondkfcheung reviewed Feb 13, 2025

View reviewed changes

polkadot/xcm/pallet-xcm/src/lib.rs Show resolved Hide resolved

raymondkfcheung reviewed Feb 13, 2025

View reviewed changes

raymondkfcheung self-assigned this Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EventEmitter to XCM executor. #7234

Add EventEmitter to XCM executor. #7234

jpserrat commented Jan 18, 2025 •

edited

Loading

bkontur Jan 20, 2025

acatangiu Jan 20, 2025

bkontur Jan 20, 2025

bkontur Jan 20, 2025

acatangiu Jan 20, 2025

acatangiu Jan 20, 2025

acatangiu Jan 20, 2025

jpserrat Feb 11, 2025

acatangiu Feb 12, 2025

acatangiu Jan 20, 2025

acatangiu Jan 20, 2025

acatangiu Jan 20, 2025

jpserrat Feb 11, 2025

acatangiu Feb 12, 2025

acatangiu Jan 20, 2025

franciscoaguirre Jan 28, 2025

acatangiu Jan 28, 2025

acatangiu commented Feb 10, 2025

jpserrat commented Feb 10, 2025

raymondkfcheung commented Feb 11, 2025

raymondkfcheung Feb 13, 2025

raymondkfcheung Feb 13, 2025

jpserrat Feb 13, 2025

raymondkfcheung Feb 14, 2025

raymondkfcheung Feb 13, 2025

raymondkfcheung Feb 13, 2025 •

edited

Loading

raymondkfcheung Feb 13, 2025

	Self::deposit_event(Event::Sent { origin, destination, message, message_id });
	Self::deposit_event(Event::XcmExecutorSent { origin, destination, message_id });

Add EventEmitter to XCM executor. #7234

Are you sure you want to change the base?

Add EventEmitter to XCM executor. #7234

Conversation

jpserrat commented Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu commented Feb 10, 2025

jpserrat commented Feb 10, 2025

raymondkfcheung commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondkfcheung Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpserrat commented Jan 18, 2025 •

edited

Loading

raymondkfcheung Feb 13, 2025 •

edited

Loading